Build: expose VCS-related environment variables #10168

humitos · 2023-03-21T12:11:56Z

Introduce 3 new variables:

READTHEDOCS_REPOSITORY_URL
READTHEDOCS_REPOSITORY_IDENTIFIER
READTHEDOCS_REPOSITORY_IDENTIFIER_HASH

Closes #9423
Based on #10166

📚 Documentation previews 📚

User's documentation (docs): https://docs--10168.org.readthedocs.build/en/10168/

Developer's documentation (dev): https://dev--10168.org.readthedocs.build/en/10168/

This variable communicates what's the canonical URL for the version it's running the build. It will allow doctool/theme authors to implement the correct HTML tag required: ``` <link rel="canonical" href="" /> ``` Note the final `href` on each page will be `READTHEDOCS_CANONICAL_URL` + `page`. Closes readthedocs/readthedocs-ops#1320

Add it to the reference page.

Co-authored-by: Benjamin Balder Bach <[email protected]>

Introduce 3 new variables: * `READTHEDOCS_REPOSITORY_URL` * `READTHEDOCS_REPOSITORY_IDENTIFIER` * `READTHEDOCS_REPOSITORY_IDENTIFIER_HASH` Closes #9423

benjaoming

I had a look at the tests for this, and it looks solid! It's a very nice pattern to be able to expand with more environment variables 💯

I'd like the documentation to be updated with a few tweaks, but otherwise I think that these 3 environment variables are good to ship and support in the future 👍

docs/user/reference/environment-variables.rst

Co-authored-by: Benjamin Balder Bach <[email protected]>

docs/user/reference/environment-variables.rst

humitos · 2023-03-21T16:08:08Z

I think I accepted all the suggestions properly. -- let's see

benjaoming · 2023-03-22T08:25:15Z

Yes, all looks good 👍

benjaoming · 2023-03-22T08:44:35Z

Git is weird sometimes... the base used to be humitos/canonical-env and that one got merged into main -- however, I still had to resolve a merge conflict in this PR with main... and the merge conflict was a block with changes and an empty block.. a merge conflict basically without any conflicting content 🤷

benjaoming · 2023-03-22T08:45:03Z

Docs build failure = 2 warnings fixed in #10176

humitos · 2023-03-22T09:08:44Z

READTHEDOCS_REPOSITORY_URL

I just realized that this variable should probably be READTHEDOCS_REPOSITORY_CLONE_URL (project.repo) and we should have a READTHEDOCS_REPOSITORY_HTML_URL (project.remote_repository.html_url)

There are some use cases where we have the clone url, but we don't have the html one. They could be different and the clone url is not useful to generate links

benjaoming · 2023-03-22T09:31:18Z

I did suggest it earlier :)
#9423 (comment)

We will want to expose `HTML_URL` as well, but that will require some extra work since we don't have access to `RemoteRepository` from the builder. I'm keeping it commented for now, but reserving the names.

…mitos/repository-env-variables

ericholscher

I have some quibbles on the naming, but I see it's been heavily discussed already, and I didn't read the backlog 🤣 . I think the examples are 💯 for making it very explicit what we're talking about, and the names aren't hugely important.

ericholscher · 2023-03-23T23:19:17Z

docs/user/reference/environment-variables.rst

@@ -56,6 +56,31 @@ All :doc:`build processes </builds>` have the following environment variables au
    :Example: ``https://docs.readthedocs.io/ja/stable/``
    :Example: ``https://example--17.org.readthedocs.build/fr/17/``

+.. envvar:: READTHEDOCS_REPOSITORY_CLONE_URL


Is there a reason we're calling it CLONE_URL, instead of REPOSITORY_URL to match the help_text?

This is the same URL defined in your Project's :term:dashboard in :menuselection:Admin --> Settings --> Repository URL.

It's because the repository will also have a "web URL", i.e. the base URL for where the file can be viewed and edited, so ssh://[email protected]/repo is different from https://frontend.com/repo/

Seems the doc is not correct then when it says it's the same as your Repository URL in the dashboard?

Users are definitely going to be confused if it's different, but only in some cases 🙃

"Repository URL in the dashboard" is what we use as Project.repo, which is the same of the content of this variable. Do you have a suggestion how to make this clearer? I think having all these URLs is just confusing, but necessary, tho.

I want to make the distinction between HTML_URL and CLONE_URL mainly because they are not going to be always the same. In community, they will mostly be the same, tho.

ericholscher · 2023-03-23T23:22:03Z

docs/user/reference/environment-variables.rst

+    :Example: ``feature/signup``
+    :Example: ``update-readme``
+
+.. envvar:: READTHEDOCS_REPOSITORY_IDENTIFIER_HASH


I don't love how long this name is, but it seems reasonably explicit.

I think IDENTIFIER and HASH are kind of the same thing, but I'm not sure if git has a canonical name for this. I couldn't quickly find it in the docs.

I added HASH here to be explicit about a commit in particular. We are using IDENTIFIER for the branch/tag name as well. I didn't find a better name for this 😞

ericholscher · 2023-03-23T23:23:18Z

docs/user/reference/environment-variables.rst

+    :Example: ``https://github.com/readthedocs/readthedocs.org``
+    :Example: ``[email protected]:readthedocs/readthedocs.org.git``
+
+.. envvar:: READTHEDOCS_REPOSITORY_IDENTIFIER


Is this a repository id? Or a version ID?

This is probably the name I'm the most unclear on whether it's correct.

Again though, perfect naming is hard, and the examples tell most of the story.

I like READTHEDOCS_GIT_IDENTIFIER better. In fact I like everything better with git :)

READTHEDOCS_GIT_CLONE_URL
READTHEDOCS_GIT_COMMIT_HASH
READTHEDOCS_GIT_IDENTIFIER

it's easier to understand. I think the old VCS semi-supported backends can just have to deal with that. If we want to really support something, we might as well re-introduce "VCS" or have stuff like READTHEDOCS_HG_CLONE_URL... but that's not relevant now.

Yea, if we wanted it to be generic, we could use VCS, but I agree GIT is probably better, and forward looking 👍

OK, I'm going to change the names to use GIT 👍🏼

…mitos/repository-env-variables

humitos · 2023-03-27T10:04:45Z

the names aren't hugely important

I'd like to think a little more if necessary, since this is something we will need to maintain for a long time 😄 . However, it seems with the latest updates, we are all in the same page and the names communicate better what we are talking about.

We need to figure it out to expose READTHEDOCS_GIT_HTML_URL. However, I didn't want to block this PR on it because that will require a modification in the API and I'm not sure yet how to do it.

ericholscher

These names are much more explicit with GIT in them 💯

We need to figure it out to expose READTHEDOCS_GIT_HTML_URL. However, I didn't want to block this PR on it because that will require a modification in the API and I'm not sure yet how to do it.

Can we copy GitHub here with naming? Is that what we're doing?

"url": "https://api.github.com/repos/octokit/octokit.rb/contents/README.md",
"git_url": "https://api.github.com/repos/octokit/octokit.rb/git/blobs/3d21ec53a331a6f037a91c368710b99387d012c1",
"html_url": "https://github.com/octokit/octokit.rb/blob/master/README.md",
"download_url": "https://raw.githubusercontent.com/octokit/octokit.rb/master/README.md",

Via: https://docs.github.com/en/rest/repos/contents?apiVersion=2022-11-28#get-a-repository-readme

I couldn't find these names explained anywhere though 🙃 Naming is hard.

ericholscher · 2023-03-27T21:20:21Z

docs/user/reference/environment-variables.rst

@@ -56,6 +56,31 @@ All :doc:`build processes </builds>` have the following environment variables au
    :Example: ``https://docs.readthedocs.io/ja/stable/``
    :Example: ``https://example--17.org.readthedocs.build/fr/17/``

+.. envvar:: READTHEDOCS_GIT_CLONE_URL


This is clearer 👍

humitos · 2023-03-28T08:33:08Z

Can we copy GitHub here with naming? Is that what we're doing?

Yes, something like that is what we are trying to follow. Hence the _URL and _CLONE urls.

comzyh · 2023-11-19T11:18:50Z

readthedocs/projects/tests/test_build_tasks.py

@@ -279,6 +279,9 @@ def test_get_env_vars(self, load_yaml_config, build_environment, config, externa
            "READTHEDOCS_OUTPUT": os.path.join(
                self.project.checkout_path(self.version.slug), "_readthedocs/"
            ),
+            "READTHEDOCS_GIT_CLONE_URL": self.project.repo,
+            "READTHEDOCS_GIT_IDENTIFIER": self.version.identifier,
+            "READTHEDOCS_GIT_COMMIT_HASH": self.build.commit,


Is it possible that the self.build.commit is None here?
And subprocess.popen doesn't accept a None value for environment variables?

It's possible that some of these values may be None if there is an error, we use docker, not sure if that also has a problem with None. Are you hitting this problem somwhere?

humitos and others added 9 commits March 21, 2023 10:56

Docs: document READTHEDOCS_CANONICAL_URL

d3f1bff

Add it to the reference page.

Docs: add another example

39c1fd3

Test: check the READTHEDOCS_CANONICAL_URL is present

58c49c3

Apply suggestions from code review

04088d5

Co-authored-by: Benjamin Balder Bach <[email protected]>

Update docs/user/reference/environment-variables.rst

5fde482

Co-authored-by: Benjamin Balder Bach <[email protected]>

Docs: fix references

5bb3d74

Test: check for canonical_url on Version endpoint

4687f09

Build: expose VCS-related environment variables

3114f95

Introduce 3 new variables: * `READTHEDOCS_REPOSITORY_URL` * `READTHEDOCS_REPOSITORY_IDENTIFIER` * `READTHEDOCS_REPOSITORY_IDENTIFIER_HASH` Closes #9423

humitos requested review from a team as code owners March 21, 2023 12:11

humitos requested a review from benjaoming March 21, 2023 12:11

auto-assign bot assigned humitos Mar 21, 2023

benjaoming approved these changes Mar 21, 2023

View reviewed changes

Apply suggestions from code review

0ffc99a

Co-authored-by: Benjamin Balder Bach <[email protected]>

humitos commented Mar 21, 2023

View reviewed changes

docs/user/reference/environment-variables.rst Outdated Show resolved Hide resolved

Update docs/user/reference/environment-variables.rst

8432ef5

Base automatically changed from humitos/canonical-env to main March 22, 2023 08:26

Merge branch 'main' into humitos/repository-env-variables

420ba97

Build: rename the env variable to use CLONE_URL

4b2eb75

We will want to expose `HTML_URL` as well, but that will require some extra work since we don't have access to `RemoteRepository` from the builder. I'm keeping it commented for now, but reserving the names.

humitos requested a review from ericholscher March 23, 2023 14:16

Merge branch 'main' of github.com:readthedocs/readthedocs.org into hu…

6d4dd3b

…mitos/repository-env-variables

ericholscher approved these changes Mar 23, 2023

View reviewed changes

ericholscher reviewed Mar 23, 2023

View reviewed changes

humitos added 2 commits March 27, 2023 11:58

Merge branch 'main' of github.com:readthedocs/readthedocs.org into hu…

77af82e

…mitos/repository-env-variables

Build: rename env variables to use GIT on their names

735d21b

ericholscher approved these changes Mar 27, 2023

View reviewed changes

humitos merged commit 8dd6732 into main Mar 28, 2023

humitos deleted the humitos/repository-env-variables branch March 28, 2023 08:33

comzyh reviewed Nov 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: expose VCS-related environment variables #10168

Build: expose VCS-related environment variables #10168

humitos commented Mar 21, 2023 •

edited by github-actions bot

Loading

benjaoming left a comment

humitos commented Mar 21, 2023

benjaoming commented Mar 22, 2023

benjaoming commented Mar 22, 2023

benjaoming commented Mar 22, 2023

humitos commented Mar 22, 2023

benjaoming commented Mar 22, 2023

ericholscher left a comment •

edited

Loading

ericholscher Mar 23, 2023

benjaoming Mar 23, 2023

ericholscher Mar 23, 2023 •

edited

Loading

humitos Mar 27, 2023

ericholscher Mar 23, 2023

humitos Mar 27, 2023

ericholscher Mar 23, 2023 •

edited

Loading

benjaoming Mar 23, 2023

ericholscher Mar 24, 2023

humitos Mar 27, 2023

humitos commented Mar 27, 2023

ericholscher left a comment •

edited

Loading

ericholscher Mar 27, 2023

humitos commented Mar 28, 2023

comzyh Nov 19, 2023

stsewd Nov 22, 2023

Build: expose VCS-related environment variables #10168

Build: expose VCS-related environment variables #10168

Conversation

humitos commented Mar 21, 2023 • edited by github-actions bot Loading

benjaoming left a comment

Choose a reason for hiding this comment

humitos commented Mar 21, 2023

benjaoming commented Mar 22, 2023

benjaoming commented Mar 22, 2023

benjaoming commented Mar 22, 2023

humitos commented Mar 22, 2023

benjaoming commented Mar 22, 2023

ericholscher left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Mar 27, 2023

ericholscher left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Mar 21, 2023 •

edited by github-actions bot

Loading

ericholscher left a comment •

edited

Loading

ericholscher Mar 23, 2023 •

edited

Loading

ericholscher Mar 23, 2023 •

edited

Loading

ericholscher left a comment •

edited

Loading